@jwnimmer-tri reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @SeanCurtis-TRI)

src/index.js line 371 at r1 (raw file):

        // setting a value will automatically trigger a re-render. When we call
        // set_property() on the containing SceneNode, we need to trigger a
        // rerender. SceneNode.set_property() calls this function, to indicate

typo (consistency)

Suggestion:

re-render.

src/index.js line 1131 at r1 (raw file):

    const extension = path.slice((path.lastIndexOf('.') - 1 >>> 0) + 2);
    let texture = null;
    switch (extension.toLowerCase()) {

The path here is... a URL? I don't think we can trust that URLs end with human extensions. The drake::geometry::Meshcat CAS cache doesn't set the extention, does it? Only the mime type.

src/index.js line 1391 at r1 (raw file):

        this.update_background();

        this.set_object(["Render Settings"], new RenderConfig());

This causes a console error message.

test/background.html line 30 at r1 (raw file):

            // use_hdr defaults to false; so env map defaults to png.
            let env_map = env_map_png;
            

nit Trailing whitespace.

test/background.html line 52 at r1 (raw file):

                    }],
                    materials: [
                        { 

nit Trailing whitespace

test/background.html line 135 at r1 (raw file):

                        regardless of camera projection type or definition of \
                        an environment map. The white field still serves as \
                        environment map, so the box should be brightly lit."

typo

Suggestion:

sphere

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @SeanCurtis-TRI)

src/index.js line 1131 at r1 (raw file):

(Also if its a URL it could have ? or # trailing the filename extension, I think.)

Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @jwnimmer-tri)

src/index.js line 1131 at r1 (raw file):

I think you're half right.

drake:geometry::Meshcat doesn't include extension. But it also doesn't include mime type. It's strictly the sha.

So, I'll have to try something more robust. Stay tuned.

I ended up going with the cascading parsers. By reversing the parser order, things worked (the TextureLoader() is apparently better than the RGBELoader(). So, I didn't have to play the game of sniffing types from urls.

src/index.js line 1391 at r1 (raw file):

Done.

@jwnimmer-tri reviewed 1 of 3 files at r2, all commit messages.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI)

src/index.js line 1131 at r1 (raw file):

But it also doesn't include mime type.

Right. But it's supposed to be setting a mime type.

My specific proposal was to change meshcat.cc to sniff the image and then set a mime type.

Possibly the cascading hack is sufficient to land this without changing that (working -- I'll play with things locally and work on understanding the new code), but I at least wanted to clear up my proposal.

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @jwnimmer-tri)

src/index.js line 1131 at r1 (raw file):

BTW I'm going to quickly knock out the Drake PR that broadcasts an hdr image to meshcat....I'm going to change this back to discussing until I have that proof of life.

@jwnimmer-tri reviewed 1 of 3 files at r2, all commit messages.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI)

src/index.js line 1131 at r1 (raw file):

What's the "it" in that sentence?

Our meshcat.cc http server, when it replies to a CAS cache request, should be returning a Content-Type in the header along with the payload. The FileStorage class should offer the option to assign mime type to each entry.

Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @SeanCurtis-TRI)

src/index.js line 1131 at r1 (raw file):

The FileStorage class should offer the option to assign mime type to each entry.

Or perhaps more tactically -- the MemoryFile already stores "extension". So we could just lean on that, and have meshcat.cc map the extension to a Content-Type during serving. The FileStorage doesn't strictly need to know about mime types, I suppose.

src/index.js line 1140 at r3 (raw file):

}

// Load a new environment map (from the given url).

nit verb mood

Suggestion:

Loads

First pass maybe complete? The control flow is difficult. I'll do a second pass for clarity after all of the posted clarity defects are fixed.

@jwnimmer-tri reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @SeanCurtis-TRI)

src/index.js line 1131 at r1 (raw file):

I think the current load_env_texture logic of "throw it at all loaders and see what sticks" is simple enough that we can live with it for now.

Consider if there is any TODO for future improvement worth adding here, while it's fresh in your head.

src/index.js line 424 at r3 (raw file):

    // should only be called asynchronously if a texture has been successfully
    // loaded.
    set_environment_texture(url, texture, scene, is_visible, is_perspective) {

The is_visible argument is dead. We should remove it.

src/index.js line 432 at r3 (raw file):

    // Clears the environment texture (if any).
    clear_environment_texture(scene, is_perspective) {

Do we really need separate "set" and "clear" functions? It seems to me like calling "set" and passing url=null and texture=null would still be readable and correct.

src/index.js line 523 at r3 (raw file):

        } else {
            // Note: clear_environment_texture() updates the state and calls
            // _configure_environment_map_from_state() for us. 

nit Trailing whitespace.

src/index.js line 1144 at r3 (raw file):

// any) is currently loaded.)
// TODO(SeanCurtis-TRI): This should only be called by background.update().
//   Refactor this so that requirement is more strongly enforced.

nit The plumbing of is_visible and is_perspective through the loading and callbacks is comprehensively terrible. Please add more specifics to the TODO here to directly state that we need to murder all of that plumbing.

src/index.js line 1147 at r3 (raw file):

function load_env_texture(url, background, scene, is_visible, is_perspective,
                          signal_complete) {
    // Action we take on successfully loading a texture (of either flavor).

nit "either flavor" here is a bit abrupt. We haven't explained yet that there are low- and high- dynamic range textures, which we need to handle completely separately. Probably the function API comment above is a good place to make that introduction?

src/index.js line 1148 at r3 (raw file):

                          signal_complete) {
    // Action we take on successfully loading a texture (of either flavor).
    let on_load = (texture) => {

The control flow readability here can be vastly improved: jwnimmer-tri@e5e4dd0

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @SeanCurtis-TRI)

src/index.js line 1148 at r3 (raw file):

(To be clear, I didn't test my patch at all. Treat it as an illustration of the new idea, to be fine-tuned locally.)

Comments addressed.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SeanCurtis-TRI)

src/index.js line 1144 at r3 (raw file):

The remnants of is_visible were an oversight and it was straightforward to give is_perspective the same treatment.

but I haven't tested locally yet. I'll do that once all discussions are resolved (so I can test the final version of the compiled dist file).

@jwnimmer-tri reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SeanCurtis-TRI)

src/index.js line 417 at r4 (raw file):

            "render_map": null,
            "visible": true,
            "is_perspective": true,

nit The trailing comma here seems non-javascriptian.

src/index.js line 432 at r4 (raw file):

            log.warning("Attempting to set the environment texture with " +
                `url = ${url} and texture = ${texture}, but both must be ` +
                "null or non-null together. Clearing the environment map.");

nit Isn't this dead code?

src/index.js line 1146 at r4 (raw file):

// TODO(SeanCurtis-TRI): This should only be called by background.update().
//   Refactor this so that requirement is more strongly enforced.
function load_env_texture(background, scene, signal_complete) {

nit This seems to be the first use of "signal" in this file. Perhaps we could use a more familiar term in this ecosystem, like "on_complete" or "on_success_or_error"?

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jwnimmer-tri)

src/index.js line 432 at r4 (raw file):

Dead in what sense? If this function gets called with them unmatching, this would get triggered. It's feedback against misuse.

As I see it, either I'm going to assume that the parameters are consistent, or I'm not. If I'm not going to assume the parameters are consistent, then I'm going to broadcast a warning that the function was misused.

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI)

src/index.js line 432 at r4 (raw file):

Dead in the sense that no possible message from the websocket can cause this code to be run.

Users don't call these functions, only we do.

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @jwnimmer-tri)

src/index.js line 432 at r4 (raw file):

That's what I suspected. I've removed it and amended the docs.

This seems to need a rebase onto the latest master.

@jwnimmer-tri reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SeanCurtis-TRI)

@jwnimmer-tri reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI)

a discussion (no related file):
BTW From the lastest push, I ran background.html in chromium. The "use hdr" checkbox doesn't seem to do anything... unless I'm holding it wrong?

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SeanCurtis-TRI)

a discussion (no related file):

Derp, nevermind. I forgot to run the http server and was running it as a file url instead, so of course the hdr file wasn't working. All good now.

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @jwnimmer-tri)

a discussion (no related file):

I just modified it so that the UseHDR control is only available if UseEnvironmentMap is enabled. That will also forestall issues in the future.

(To close this out, the rebase has happened.)

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @jwnimmer-tri)

@jwnimmer-tri reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SeanCurtis-TRI)